Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Partially remove db exist check if ignore db is false #17300

Merged
merged 18 commits into from
Apr 4, 2024
Merged

Partially remove db exist check if ignore db is false #17300

merged 18 commits into from
Apr 4, 2024

Conversation

kozlovb
Copy link
Contributor

@kozlovb kozlovb commented Mar 27, 2024

What does this PR do?

Removes the database exist check in sql_integration if ignore_missing_database is set to false

Motivation

The database exist check got introduced in #357 for the reason that we don't see as valid anymore. The check is error prone and causes false positive. Nevertheless in order to avoid a breaking change we dont modify the behaviour for users that set ignore_missing_database to true. For user who set it to False we remove the check as anyway the exception when connecting to database will be thrown.

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • Changelog entries must be created for modifications to shipped code
  • Add the qa/skip-qa label if the PR doesn't need to be tested during QA.

@kozlovb kozlovb requested review from a team as code owners March 27, 2024 22:50
@kozlovb kozlovb marked this pull request as draft March 28, 2024 02:14
@kozlovb kozlovb added the qa/skip-qa Automatically skip this PR for the next QA label Mar 28, 2024
@kozlovb kozlovb force-pushed the BorisKozlov/PartiallyRemoveDBExistCheck-DBMON-3737 branch from 6fe9b50 to e95e8ca Compare March 28, 2024 10:01
@kozlovb kozlovb marked this pull request as ready for review March 28, 2024 13:04
@github-actions GitHub Actions
Copy link

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

@codecov Codecov
Copy link

codecov bot commented Mar 28, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 92.30769% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 90.26%. Comparing base (701a29e) to head (2991646).

Additional details and impacted files
Flag Coverage Δ
activemq ?
cassandra ?
hive ?
hivemq ?
hudi ?
ignite ?
jboss_wildfly ?
kafka ?
presto ?
solr ?
sqlserver 88.67% <92.30%> (-1.30%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 28, 2024 •
edited
Loading

Test Results

   20 files     20 suites   1h 17m 47s ⏱️
  279 tests   267 ✅  12 💤 0 ❌
3 936 runs  3 616 ✅ 320 💤 0 ❌

Results for commit 2991646.

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

5 similar comments
@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 2, 2024

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 2, 2024

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 2, 2024

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 2, 2024

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 2, 2024

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

lu-zhengda
lu-zhengda reviewed Apr 2, 2024
View reviewed changes
sqlserver/datadog_checks/sqlserver/data/conf.yaml.example Outdated
@@ -22,7 +22,6 @@ init_config:
# - query: <QUERY>
# columns: <COLUMNS>
# tags: <TAGS>
# collection_interval: <COLLECTION_INTERVAL>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of the checks failed and asked me to run some script that aligns this file with config. We could also split in 2 PRs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

try rebase with master and run ddev validate config sqlserver -s

sqlserver/datadog_checks/sqlserver/sqlserver.py Outdated
except ConfigurationError:
if self._config.ignore_missing_database:
# self.connection.check_database() will try to connect to 'master'.
# If this is a DB hosted on Azure the function would throw.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DB hosted on Azure is too vague. Let's be specific that the issue is for Azure SQL Database.

sqlserver/datadog_checks/sqlserver/sqlserver.py Outdated
Comment on lines 967 to 977
with self.connection.open_managed_default_connection():
with self.connection.get_managed_cursor() as cursor:
try:
self.log.debug("Calling Stored Procedure : %s", proc)
if self.connection.connector == 'adodbapi':
cursor.callproc(proc)
else:
# pyodbc does not support callproc; use execute instead.
# Reference: https://github.com/mkleehammer/pyodbc/wiki/Calling-Stored-Procedures
call_proc = '{{CALL {}}}'.format(proc)
cursor.execute(call_proc)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change required for this PR? if it's a cleanup, let's open another PR to minimize the changes

@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 3, 2024

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

1 similar comment
@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 3, 2024

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

@kozlovb kozlovb force-pushed the BorisKozlov/PartiallyRemoveDBExistCheck-DBMON-3737 branch from da08d7d to 2991646 Compare April 3, 2024 16:11
lu-zhengda
lu-zhengda approved these changes Apr 3, 2024
View reviewed changes
sqlserver/datadog_checks/sqlserver/data/conf.yaml.example Outdated
@@ -22,7 +22,6 @@ init_config:
# - query: <QUERY>
# columns: <COLUMNS>
# tags: <TAGS>
# collection_interval: <COLLECTION_INTERVAL>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

try rebase with master and run ddev validate config sqlserver -s

@kozlovb kozlovb merged commit ddea535 into master Apr 4, 2024
36 checks passed
@kozlovb kozlovb deleted the BorisKozlov/PartiallyRemoveDBExistCheck-DBMON-3737 branch April 4, 2024 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lu-zhengda lu-zhengda lu-zhengda approved these changes

Assignees
No one assigned
Labels
documentation integration/sqlserver qa/skip-qa Automatically skip this PR for the next QA team/agent-integrations team/database-monitoring-agent
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kozlovb @lu-zhengda